Simplify SGB related UIs and make it possible to use SGB under Gambatte link #3635

Fortranm · 2023-04-16T21:16:32Z

This is a revision of #3283 made to work on BizHawk 2.9.

https://en.wikipedia.org/wiki/Super_Game_Boy#Super_Game_Boy_2

The Super Game Boy 2 was released exclusively in Japan in 1998. Additions included a link port to allow a user to access two-player mode via the link cable...

Currently, the SGB mode of Gambatte has to be turned on via Config > Cores > GB in SGB instead of in the sync setting and cannot be used in GB Link as a result.

This PR does three things related to SGB:

SGB is added (or rather, merged in) as a Console Mode option for Gambatte.
"GB in SGB" check box and SGB core picker are removed and the BSNES cores are added under the GB core picket (legacy BSNES core not included). GB/GBC games will load under SGB mode of the corresponding BSNES core if one of those options are chosen.
As a result of the first change (with addition adjustments), it is now possible to use SGB mode in Gambatte Link too.

Each SGB core can have up to 4 controllers linked to it. The Power button for each SGB core is included on the first Player tab of the core for the time being.
Note that the tab parsing process is currently hardcoded to categorize every item that starts with "Px " under the tab "Player x" and everything else under "Console". There have been suggestions to use "Console x" for a core in link mode but that requires enough refactoring to be considered a separate task IMO.

Note: The SGB sound is not implemented yet but that can be done along with the MBC sound fix for Gambatte Link altogether later.

resolves #2289

src/BizHawk.Emulation.Cores/Consoles/Nintendo/BSNES/BsnesCore.cs

src/BizHawk.Emulation.Cores/Consoles/Nintendo/Gameboy/GambatteLink.IEmulator.cs

src/BizHawk.Client.Common/movie/bk2/Bk2MnemonicLookup.cs

src/BizHawk.Emulation.Cores/Consoles/Nintendo/Gameboy/GambatteLink.IVideoProvider.cs

src/BizHawk.Emulation.Cores/Consoles/Nintendo/Gameboy/GambatteLink.cs

TiKevin83

approving from a code perspective, haven't done a QA of the functionality yet

src/BizHawk.Client.Common/RomLoader.cs

src/BizHawk.Client.Common/config/Config.cs

src/BizHawk.Client.Common/movie/import/LsmvImport.cs

Morilli

Haven't tested the gambatte changes extensively but I think this is solid enough to get in. The "gb as sgb" UX flow was pretty bad imo so this is a definite improvement.

YoshiRulz

I'm curious of what @CasualPokePlayer has to say, specifically about the video buffer stuff. But this now LGTM in terms of code. (I think you deserve some kind of trophy for hacking together the button mapping by doing it in the wrong direction and with 3 nested loops.)
~~As for functionality, opening a 4x bundle and immediately closing it results in:~~

stacktrace

System.NullReferenceException: Object reference not set to an instance of an object
  at BizHawk.Emulation.Common.LinkedSaveRam.CloneSaveRam () [0x0002f] in <f3a4da15026e4f05ab7072e9a135677c>:0
  at BizHawk.Client.EmuHawk.MainForm.FlushSaveRAM (System.Boolean autosave) [0x000f0] in <5e7f3991f9b246bb80e8d9725eda76e1>:0
  at (wrapper remoting-invoke-with-check) BizHawk.Client.EmuHawk.MainForm.FlushSaveRAM(bool)
  at BizHawk.Client.EmuHawk.MainForm.CloseGame (System.Boolean clearSram) [0x0006f] in <5e7f3991f9b246bb80e8d9725eda76e1>:0
  at BizHawk.Client.EmuHawk.MainForm.CloseRom (System.Boolean clearSram) [0x00010] in <5e7f3991f9b246bb80e8d9725eda76e1>:0
  at (wrapper remoting-invoke-with-check) BizHawk.Client.EmuHawk.MainForm.CloseRom(bool)
  at BizHawk.Client.EmuHawk.MainForm.CheckHotkey (System.String trigger) [0x01588] in <5e7f3991f9b246bb80e8d9725eda76e1>:0
  at BizHawk.Client.EmuHawk.MainForm.<ProcessInput>b__143_0 (System.Boolean current, System.String trigger) [0x00000] in <5e7f3991f9b246bb80e8d9725eda76e1>:0
  at System.Linq.Enumerable.Aggregate[TSource,TAccumulate] (System.Collections.Generic.IEnumerable`1[T] source, TAccumulate seed, System.Func`3[T1,T2,TResult] func) [0x0002e] in <89a8dc5bc33447a4a98f23a2d52e8aee>:0
  at BizHawk.Client.EmuHawk.MainForm.ProcessInput (BizHawk.Client.Common.InputCoalescer hotkeyCoalescer, BizHawk.Client.Common.ControllerInputCoalescer finalHostController, System.Func`2[T,TResult] searchHotkeyBindings, System.Func`2[T,TResult] activeControllerHasBinding) [0x00135] in <5e7f3991f9b246bb80e8d9725eda76e1>:0
  at BizHawk.Client.EmuHawk.MainForm.ProgramRunLoop () [0x0009c] in <5e7f3991f9b246bb80e8d9725eda76e1>:0
  at (wrapper remoting-invoke-with-check) BizHawk.Client.EmuHawk.MainForm.ProgramRunLoop()
  at BizHawk.Client.EmuHawk.Program.SubMain (System.String[] args) [0x0042b] in <5e7f3991f9b246bb80e8d9725eda76e1>:0

YoshiRulz · 2024-06-01T07:22:46Z

I still don't have a nice repro, but I found the culprit with print-debugging, and it wasn't related to this PR. Pushed 64db1fe.
In working on that, I've seen that SGB in GambatteLink is working, so I won't block this any further. @CasualPokePlayer?

src/BizHawk.Emulation.Cores/Consoles/Nintendo/Gameboy/Gambatte.ISettable.cs

src/BizHawk.Emulation.Cores/Consoles/Nintendo/Gameboy/Gambatte.cs

CasualPokePlayer · 2024-06-19T03:02:21Z

src/BizHawk.Emulation.Cores/Consoles/Nintendo/Gameboy/Gambatte.cs

@@ -22,8 +22,7 @@ public partial class Gameboy : IInputPollable, IRomInfo, IGameboyCommon, ICycleT

 		[CoreConstructor(VSystemID.Raw.GB)]
 		[CoreConstructor(VSystemID.Raw.GBC)]
-		[CoreConstructor(VSystemID.Raw.SGB)]


Why is SGB being removed here? BSNES/BSNESv115 still have it.

Because for core choosing, the system is only set to SGB when the core is bsnes:

BizHawk/src/BizHawk.Client.Common/RomLoader.cs

Lines 485 to 490 in 9ee3830

static bool IsPreferredCoreSGB(Config config)

=> config.PreferredCores[VSystemID.Raw.GB] is CoreNames.Bsnes or CoreNames.Bsnes115 or CoreNames.SubBsnes115;

if (IsPreferredCoreSGB(_config))

{

game.System = VSystemID.Raw.SGB;

}

Thinking about it, that's probably logic that could get removed too...

How does this exactly end up working for say, loading a GBC only ROM into BSNES. Does it proceed to fail completely due to not finding a fallback with the SGB system ID ctor, or does it end up still picking Gambatte with the GBC system ID?

That's a good point. It does NOT work currently for that exact reason and instead runs into this code which then no longer makes sense:

BizHawk/src/BizHawk.Client.Common/RomLoader.cs

Line 859 in 9ee3830

DoLoadErrorCallback("Failed to load a GB rom in SGB mode. You might try disabling SGB Mode.", system);

There's more to this, and there is already broken behavior currently. I'll make an issue later.

#3960
The new issue in question for reference. The way this currently is makes sense with that proposed change.
Resolving.

Not resolved.

src/BizHawk.Emulation.Cores/Consoles/Nintendo/Gameboy/GambatteLink.IEmulator.cs

The system id will still be SGB for BSNES cores by default

This fixes failures in CorePickerStabilityTests (https://ci.appveyor.com/project/zeromus/bizhawk-udexo/builds/50119675). It's not necessary to put SGB there as the system id is only ever switched to SGB after a relevant core is already initiated.

Outdated

quicknes/core

Outdated

YoshiRulz · 2024-07-04T12:16:32Z

src/BizHawk.Emulation.Cores/Consoles/Nintendo/BSNES/BsnesCore.cs

 		[CoreConstructor(VSystemID.Raw.Satellaview)]
-		[CoreConstructor(VSystemID.Raw.SGB)]


Keep this attribute...

YoshiRulz · 2024-07-04T12:16:40Z

src/BizHawk.Emulation.Cores/Consoles/Nintendo/BSNES/SubBsnesCore.cs

 		[CoreConstructor(VSystemID.Raw.Satellaview)]
-		[CoreConstructor(VSystemID.Raw.SGB)]


...and here...

YoshiRulz · 2024-07-04T12:17:07Z

src/BizHawk.Emulation.Cores/Consoles/Nintendo/Gameboy/Gambatte.cs

@@ -21,8 +21,7 @@ public partial class Gameboy : IInputPollable, IRomInfo, IGameboyCommon, ICycleT

 		[CoreConstructor(VSystemID.Raw.GB)]
 		[CoreConstructor(VSystemID.Raw.GBC)]
-		[CoreConstructor(VSystemID.Raw.SGB)]


...and here...

YoshiRulz · 2024-07-04T12:17:29Z

src/BizHawk.Emulation.Cores/Consoles/Nintendo/SNES/LibsnesCore.cs

@@ -22,7 +22,8 @@ namespace BizHawk.Emulation.Cores.Nintendo.SNES
 	public unsafe partial class LibsnesCore : IEmulator, IVideoProvider, ISaveRam, IStatable, IInputPollable, IRegionable, ICodeDataLogger,
 		IDebuggable, ISettable<LibsnesCore.SnesSettings, LibsnesCore.SnesSyncSettings>, IBSNESForGfxDebugger
 	{
-		[CoreConstructor(VSystemID.Raw.SGB)]


...and here.

At the very least this one is questionable as the core doesn't ever actually sets its SystemID to SGB. I don't really mind this too much, but I do think we should be able to agree on what this attribute is supposed to mean.

Semantically, it indicates which constructor to use for a given RomGame.
Also, the set across all of the core's constructors doubles as the list of sysIDs which that core can emulate. We could split that out to the [Core] attr instead, but I think a single-source-of-truth is better.

BizHawk/src/BizHawk.Emulation.Cores/Consoles/Nintendo/SNES/LibsnesCore.cs

Lines 112 to 115 in dbc6acc

if (game.System == VSystemID.Raw.SGB)

{

IsSGB = true;

SystemId = VSystemID.Raw.SNES;

^ This is clearly incorrect. I imagine it was a hack to get the frontend to show the right menus/tools.

the set across all of the core's constructors doubles as the list of sysIDs which that core can emulate.

This is precisely the issue; I don't think this invariant necessarily needs to hold true (it doesn't even right now with that old bsnes core), and even with your suggested change bsnes* core cannot provide the GB/C SystemID, even though they have a corresponding CoreConstructor attribute.

That attribute is currently entirely internal and only used for the internal selection process of which cores to consider for a given rom, so putting semantic stuff here that is supposed to communicate meaning to the reader while doing nothing for the core or romloader that's using it is misguided imho.
It's fine to communicate this SGB stuff with a comment, but the attribute itself doesn't need to be here.

The conceptual question here is whether it is acceptable for a core to load a rom of type X and declare a SystemID of type Y != X using that loaded rom, and I say yes.

How about this:

RomLoader never sets SGB on any rom (since any GB rom could be used with SGB, they should all be detected as the same, and GB/GBC is the better choice)

There is no core preference for SGB, only GB and maybe GBC

Cores are perfectly honest with setting IEmulator.SystemId (not necessarily matching the one passed in, since a sync setting can choose between GB/GBC/SGB)

SystemId always matches a [CoreConstructor]—if it doesn't make sense to have that ctor, like with SGB which RomLoader will never use, it can simply be private and call something like this(default)

I think that would give the consistency I'm after while being simpler for regular users.

The first three should already be present here.

If you feel like adding dummy constructors with the SGB attribute just to signal intent, fine.

The first 3 are not already implemented. This PR addresses some of them, but I'm saying we should clean it up on master first.

The first 2 definitely are included in this the PR.
The only one that doesn't currently fit 3 is the legacy BSNES core. Actually modifying that core to use the SGB sysID should be a separate task imo (and hopefully the core is retired before that has to be done, maybe)

YoshiRulz mentioned this pull request Apr 26, 2023

[GameBoy Link] Buzzing noise when one of the Players is muted #3648

Closed

YoshiRulz requested a review from CasualPokePlayer June 22, 2023 15:22

YoshiRulz requested changes Jun 22, 2023

View reviewed changes

src/BizHawk.Emulation.Cores/Consoles/Nintendo/BSNES/BsnesCore.cs Show resolved Hide resolved

src/BizHawk.Emulation.Cores/Consoles/Nintendo/Gameboy/GambatteLink.IEmulator.cs Outdated Show resolved Hide resolved

Fortranm force-pushed the SGB-link branch from 734c83e to 12f5585 Compare June 23, 2023 19:58

YoshiRulz self-requested a review June 23, 2023 22:17

TiKevin83 requested changes Jul 12, 2023

View reviewed changes

TiKevin83 previously approved these changes Jul 12, 2023

View reviewed changes

Morilli reviewed Jul 13, 2023

View reviewed changes

src/BizHawk.Client.Common/RomLoader.cs Outdated Show resolved Hide resolved

src/BizHawk.Client.Common/config/Config.cs Outdated Show resolved Hide resolved

src/BizHawk.Client.Common/movie/import/LsmvImport.cs Outdated Show resolved Hide resolved

Morilli approved these changes May 29, 2024

View reviewed changes

YoshiRulz previously requested changes May 30, 2024

View reviewed changes

This comment was marked as resolved.

Sign in to view

CasualPokePlayer requested changes Jun 19, 2024

View reviewed changes

Fortranm and others added 16 commits June 30, 2024 01:09

merge SGB into a mode option of Gambatte

dd14f8e

merge the core choices for SGB

5f95a93

GBL compatibility

f8b294f

rename SGB mode to SGB2 for Gambatte and fix BSNES initialization

eecee30

SGB border in GBL

dd6abbc

adjust sgb core detection

7dd40dd

reformat controller button naming

511f48d

adjust for 2.9

2e6f59b

adjust for 2.9 again

2e534a9

revert changes to BSNES CoreConstructor

ff268f3

The system id will still be SGB for BSNES cores by default

one Player tab per SNES controller for SGB

c183e6e

idiomatic rephrase for showAnyBorder

0a3a727

no dummy for CreateSGBVideoBuffer

ef87a48

fix SGB conversion in LsmvImport

c0ff230

add legacy BSNES option for (S)GB

b2a4720

minor cosmetic changes

e8aa24a

YoshiRulz and others added 5 commits June 30, 2024 02:11

Fix .vbm importer when Gambatte isn't the preferred GB core

3786258

Fix button mapping spaghetti

0f5cc8d

Code style nitpicks

b910c38

minor cleanups in Gambatte.cs

864ee92

Remove special SGB handling and mark bsnes* as GB/GBC cores

c68e9db

Fortranm force-pushed the SGB-link branch from 8a1a4c3 to c68e9db Compare June 30, 2024 09:21

Fortranm and others added 2 commits July 1, 2024 12:04

remove SGB from CorePickerUIData

c56f047

This fixes failures in CorePickerStabilityTests (https://ci.appveyor.com/project/zeromus/bizhawk-udexo/builds/50119675). It's not necessary to put SGB there as the system id is only ever switched to SGB after a relevant core is already initiated.

Un-revert submodule pull

f148272

Morilli reviewed Jul 1, 2024

View reviewed changes

quicknes/core Outdated Show resolved Hide resolved

Un-revert other submodule pulls

cddb113

Morilli mentioned this pull request Jul 2, 2024

Preferred core picker is intransparent and inflexible #3960

Open

Fortranm added 3 commits July 2, 2024 15:40

fix the crash from Toggle Link Connection

02746ef

remove inaccurate description of how Auto works for console

06496c6

Merge branch 'master' into SGB-link

3e401db

YoshiRulz requested changes Jul 4, 2024

View reviewed changes

Remove unused import

0e32693

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify SGB related UIs and make it possible to use SGB under Gambatte link #3635

Simplify SGB related UIs and make it possible to use SGB under Gambatte link #3635

Fortranm commented Apr 16, 2023 •

edited by YoshiRulz

Loading

TiKevin83 left a comment

Morilli left a comment

YoshiRulz left a comment •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

YoshiRulz commented Jun 1, 2024 •

edited

Loading

CasualPokePlayer Jun 19, 2024

Morilli Jun 19, 2024 •

edited

Loading

CasualPokePlayer Jun 19, 2024 •

edited

Loading

Morilli Jun 19, 2024 •

edited

Loading

This comment was marked as duplicate.

Morilli Jul 1, 2024

Fortranm Jul 4, 2024

YoshiRulz Jul 4, 2024

YoshiRulz Jul 4, 2024

YoshiRulz Jul 4, 2024

YoshiRulz Jul 4, 2024

YoshiRulz Jul 4, 2024

Morilli Jul 4, 2024

YoshiRulz Jul 5, 2024

Morilli Jul 5, 2024

YoshiRulz Jul 7, 2024 •

edited

Loading

Morilli Jul 7, 2024

YoshiRulz Jul 7, 2024

Fortranm Jul 7, 2024 •

edited

Loading

	static bool IsPreferredCoreSGB(Config config)
	=> config.PreferredCores[VSystemID.Raw.GB] is CoreNames.Bsnes or CoreNames.Bsnes115 or CoreNames.SubBsnes115;
	if (IsPreferredCoreSGB(_config))
	{
	game.System = VSystemID.Raw.SGB;
	}

		[CoreConstructor(VSystemID.Raw.Satellaview)]
		[CoreConstructor(VSystemID.Raw.SGB)]

	if (game.System == VSystemID.Raw.SGB)
	{
	IsSGB = true;
	SystemId = VSystemID.Raw.SNES;

Simplify SGB related UIs and make it possible to use SGB under Gambatte link #3635

Are you sure you want to change the base?

Simplify SGB related UIs and make it possible to use SGB under Gambatte link #3635

Conversation

Fortranm commented Apr 16, 2023 • edited by YoshiRulz Loading

TiKevin83 left a comment

Choose a reason for hiding this comment

Morilli left a comment

Choose a reason for hiding this comment

YoshiRulz left a comment • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

YoshiRulz commented Jun 1, 2024 • edited Loading

Choose a reason for hiding this comment

Morilli Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

CasualPokePlayer Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

Morilli Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

This comment was marked as duplicate.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YoshiRulz Jul 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fortranm Jul 7, 2024 • edited Loading

Choose a reason for hiding this comment

Fortranm commented Apr 16, 2023 •

edited by YoshiRulz

Loading

YoshiRulz left a comment •

edited

Loading

YoshiRulz commented Jun 1, 2024 •

edited

Loading

Morilli Jun 19, 2024 •

edited

Loading

CasualPokePlayer Jun 19, 2024 •

edited

Loading

Morilli Jun 19, 2024 •

edited

Loading

YoshiRulz Jul 7, 2024 •

edited

Loading

Fortranm Jul 7, 2024 •

edited

Loading